Skip to content

feat(reviewer): checksum-pinned reviewer sandbox image and launcher (PR 2 of #425) - #428

Merged
tps-flint merged 10 commits into
mainfrom
feat/425-reviewer-image
Sep 29, 2026
Merged

tps-flint merged 10 commits into
mainfrom
feat/425-reviewer-image

Conversation

@tps-anvil

@tps-anvil tps-anvil commented Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator

What this builds

PR 2 of #425: section A's image and launcher — the parts that do not depend on PR 1's plugin.

What review-build-ok means. It is advisory evidence for the reviewer, not a merge gate: CI remains the gate on every PR. The boundary this PR holds is that no credential and no host access are reachable from the review. How well a review build predicts CI is best effort — features the launcher cannot reproduce faithfully are refused, and the fidelity limits that remain are listed below. One integrity property is conditional on the host and not yet verified: the host assignment (below) is immutable to the review only if the reviewer host denies same-user process-memory writes (#436).

  • The image (docker/reviewer/Dockerfile, linux/amd64): FROM debian:bookworm-slim pinned by digest, with OpenClaw's sandbox package set (bash, ca-certificates, curl, git, jq, python3, ripgrep) plus Node, Bun, a gh with no credential of its own and the launcher's js-yaml, each at an exact version verified against a checksum from the trusted table. No entrypoint; CMD ["sleep", "infinity"]. Default HOME, TMPDIR and caches point under /tmp/review (a tmpfs in OpenClaw's run model). No credentials, no configured credential helpers.
  • The trusted runtime table (docker/reviewer/runtime-matrix.json): base digest and platform, exact runtime versions and checksums, the launcher's js-yaml pin (the version and integrity bun.lock pins), and the image matrix. Maintained here and installed on the host; the reviewed checkout can neither extend it nor choose a download source.
  • The resolver (scripts/reviewer/resolve-runtime.mjs): packageManager, engines, .nvmrc, .node-version, .bun-version, .tool-versions and every planned job's pins resolve to ONE matrix image with npm-semver range semantics, or are refused by name (invalid-declaration, ambiguous, conflicting, out-of-matrix). An out-of-matrix refusal names the missing image by its requirements, e.g. missing image: node >=25 with bun 1.3.10.
  • The planner (scripts/reviewer/ci-job.mjs): bounds the workflow (256 KiB; 50,000 YAML nodes counting every alias use; 32 levels), requires printable-ASCII keys and a pull_request trigger covering the base branch, and plans the host-named job with the jobs it needs. Every job must begin with actions/checkout and run on ubuntu-latest or ubuntu-24.04 (the labels these repositories use; CI shows ubuntu-latest resolving to ubuntu-24.04). timeout-minutes on the job and on run: steps is carried for enforcement; on a skipped uses: step it is refused (how the real action behaves at that limit cannot be reproduced). It skips checkout (first step only; fetch-depth 1 or 0), setup-bun and setup-node (an exact version, which becomes a verified pin), socketdev (firewall-free), cache (path and key required; a miss may not fail the job) and upload-artifact (path required; a missing file may not fail the job; a valid name unique in the workflow) — each only at a reviewed commit SHA (release tags confirmed, action.yml inputs read), with every input the real action needs, and only with literal values whose skip is equivalent. It refuses any other action, ref or input; a ${{ }} expression anywhere it would have to be evaluated — scripts, env values, working directories, runner labels, timeouts and every input of a skipped action, since a skipped action never evaluates it (a cache key such as ${{ runner.os }},bad, which the real action rejects, is refused); the only expressions accepted are if: conditions that are exactly success() or always(). It also refuses other if: conditions, continue-on-error, matrices, containers, non-bash shells, launcher-owned env keys, and credential-shaped or config-redirecting env (GH_TOKEN, *_TOKEN, *_SECRET, *_KEY, GIT_*, NPM_CONFIG_*, SSH_*, NODE_OPTIONS, BASH_ENV, LD_*, proxies, …).
  • The launcher (/opt/reviewer/bin/reviewer-launch; build mode takes no arguments and always builds /workspace; --self-check is the only other mode): takes the workflow, job and base branch only from the environment the host gave the sandbox at creation (REVIEWER_CI_WORKFLOW, REVIEWER_CI_JOB, REVIEWER_CI_BASE in sandbox.docker.env, read from the container init's environment) and refuses a caller that passes different values — which makes the assignment immutable to the review only if the host denies same-user process-memory writes, not yet verified (reviewer sandbox: check and enforce the same-user process isolation the launcher's assignment relies on #436); refuses symlinked lockfiles and symlinked directories in the worktree; resolves the declarations (read with size bounds, inside the worktree) and pins to ONE image and refuses unless it is THIS image; builds the child env from an allowlist with a fixed PATH; runs the image's own node and bun at fixed paths to verify their versions. Then, job by job in dependency order, with CI's gating: removes what earlier jobs of the build created; gives the job a fresh HOME/TMPDIR/cache root; refuses a worktree whose effective git configuration leaves the safe baseline — an allowlist of the keys a plain clone and an identity write; core.fsmonitor, core.hooksPath, core.pager, core.editor, protocol.*, uploadpack.*, url.*.insteadOf, credential helpers, auth headers and every other key are refused by key, never by value; the values of remote.<name>.url and .fetch are the host clone's and are not approved, only pinned — or whose repository holds hooks; refuses unless the worktree passes the clean-clone check: HEAD and every remote URL and fetch refspec unchanged since the first job, no tracked file carrying the assume-unchanged or skip-worktree bit, git status (replace objects ignored) reporting no modified, untracked or ignored path, and history shaped as fetch-depth asks (shallow one-commit without tags, or full for fetch-depth: 0) — not a byte comparison with a host-pinned commit; runs each run: step under /bin/bash --noprofile --norc -eo pipefail in its own process group after re-checking its effective environment and that its resolved working directory stays inside the worktree; kills a run: step at its timeout-minutes and the job's steps at the job's (default 360), and kills what the job left running when it ends. After every job it refuses symlinked lockfiles, symlinked directories and any lockfile change. review-build-ok only if every job in the closure ran and every step exited 0.
  • The CI job (reviewer-image): builds every matrix image and runs the A2/A3/A9 checks, with build-path fixtures run the OpenClaw way on depth-1 clones: out-of-matrix and other-image pins, a caller naming another job, a --workspace argument, impostor binaries on the caller's PATH, an enforced run: step timeout-minutes, a worktree that fails the clean-clone check, a git core.fsmonitor, a worktree git auth header and a workflow GH_TOKEN; then a9-red-proofs.sh requires the A9 checks to FAIL on five derived images carrying planted fake credentials.

Decisions

  • Scope (Flint): review-build-ok is evidence, not a gate; the PR must hold "no credential and no host access reachable from the review". CI-fidelity findings are fixed when cheap, otherwise documented and filed.
  • Platform: linux/amd64 only (the reviewer VMs are x86_64, Docker 29.1.3, OpenClaw 2026.8.1). Pinned in the Dockerfile FROM, the build script and the matrix; arm64 is out of scope.
  • Base image: debian:bookworm-slim pinned by digest and labelled as such. This deviates from spec A's "OpenClaw sandbox base", accepted because no digest-pinnable OpenClaw sandbox base is published: OpenClaw 2026.8.1's documented sandbox build is a local FROM debian:bookworm-slim (floating tag) with bash, ca-certificates, curl, git, jq, python3 and ripgrep; this image pins that base by digest and carries the same packages.
  • Run model: OpenClaw 2026.8.1 creates sandboxes as docker create --init --read-only --tmpfs /tmp --tmpfs /var/tmp --tmpfs /run --network none --cap-drop ALL --security-opt no-new-privileges --workdir /workspace … <image> sleep infinity without overriding the entrypoint. So the image has no entrypoint, and the launcher is an explicit command that creates its own hermetic directories.
  • Host binding: the assignment is read from the sandbox's creation environment, not a bind-mounted file: OpenClaw 2026.8.1 allows bind sources only inside the workspace roots, which are mounted writable, unless dangerouslyAllowExternalBindSources is set. Its integrity against the review depends on the host denying same-user process-memory writes; enforcing and testing that on the reviewer hosts is reviewer sandbox: check and enforce the same-user process isolation the launcher's assignment relies on #436, and until it lands the claim is conditional.
  • Host worktree contract: the host mounts a fresh clone of the assigned head with its git metadata inside it — depth 1 without tags for checkout's default fetch, a full clone for fetch-depth: 0.
  • timeout-minutes: enforced on the job and on run: steps, not refused (flair's workflows use it); refused on a skipped uses: step.
  • Expressions in skipped-action inputs: refused in every input, not only the required ones: restore-keys has the same failure rule as key (actions/cache rejects any key over 512 characters or containing a comma), and a skipped action never evaluates either.
  • Symlinks: lockfile symlinks and directory symlinks are refused before the build and after every job; lockfile drift is not hashed through links.
  • No registry in this slice: the host builds the image from the pinned Dockerfile at install time and records its local image id in host config (sandbox.docker.image: "sha256:…"). Builds on different hosts are not claimed to produce the same image id (apt packages are not pinned); cross-host reproducibility is a follow-up.
  • This repository declares engines.node: "22.x" in package.json, the Node major its CI runs, so it resolves to reviewer-node22-bun1310.

Tests

The cli suite runs the four reviewer test files: the planner, the resolver, the A3 diff filter and the launcher end to end with real bash and real git on depth-1 clones of a temporary source repository. Every round-7 control has a test that fails on 38ac74d and passes here. Mutation-checked: the round-2, round-5 and round-6 sets re-run plus every round-7 control (break → red; restore by copy → green); the exact table and counts are in the round-7 notes. The image-level checks, the host-binding, timeout, fresh-clone, git-baseline and credential fixtures, and the A9 red proofs run only in the reviewer-image job.

Known limits

Trust boundary (host integration, PR 3):

  • The assignment is read from the container init's environment, which runs as the same user as the sandbox. Its integrity relies on the deployed sandbox denying same-user process-memory writes (no ptrace capability; Yama ptrace_scope ≥ 1). That host-integration check is not in place (reviewer sandbox: check and enforce the same-user process isolation the launcher's assignment relies on #436): until it lands, "a caller cannot change the assignment" holds against a caller that sets its own environment, not against one that can write the init process's memory.

CI fidelity (review-build-ok may disagree with CI):

  • Jobs of a needs closure share one worktree. Before each job the launcher removes what earlier jobs created and applies the clean-clone check; what that check cannot see carries over — a change git status does not report (for example a line-ending-only change under a text attribute) and .git/ state beyond HEAD, the index bits and the remote settings (reviewer sandbox: a fresh isolated tree per needs job #435: an independent tree per job at a host-pinned commit).
  • On pull_request, CI checks out the merge of the head into the base; the review builds the assigned head.
  • CI's runner carries its own Node where a workflow does not pin one (22.23.2 today); the review uses the matrix image's (22.22.1).
  • Socket Firewall is not reproduced (sfw runs the command unwrapped); cache restores never happen (a cold build); artifact uploads are not performed.
  • Each step runs with pipefail; CI's default for an unspecified shell is bash -e {0}.
  • Only a pull_request trigger with exact branch names is evaluated; the assignment is static per reviewer, so a review into another base is refused.
  • The "no credential" statements cover the image, the environments the launcher builds and the worktree's git configuration; a credential that repository code itself supplies is outside them.
  • OpenClaw's default /tmp tmpfs is noexec; a suite that executes files from TMPDIR needs /tmp:exec in the reviewer's sandbox config (A15).

What PR 3 will add

Refs #425

🤖 Generated with Claude Code

…me table and launcher

Adds docker/reviewer: a digest-pinned sandbox image with git, tokenless gh,
Bun and Node, each installed at an exact version and verified against a
checksum from the trusted runtime table. Adds scripts/reviewer: the resolver
that reads the reviewed commit's packageManager/engines/runtime-version files
and CI lanes and resolves them to one matrix entry or refuses by name
(ambiguous, conflicting, out-of-matrix with the missing image named), and the
trusted launcher that records and verifies the actual Bun/Node versions, sets
hermetic container-local HOME/tmp/cache defaults, and runs the repository's own
frozen install, build and test commands. A CI job builds every matrix image and
runs the image-level checks (integrity, hermetic defaults, tokenless gh).
@tps-anvil
tps-anvil requested a review from a team as a code owner September 28, 2026 20:16
@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 39 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: b0481429-385b-4932-a3a2-4633c55478ee

📥 Commits

Reviewing files that changed from the base of the PR and between ac10d66 and 9df196f.

📒 Files selected for processing (19)
  • .changelog/unreleased/added-reviewer-sandbox-image.md
  • .github/workflows/test.yml
  • docker/reviewer/Dockerfile
  • docker/reviewer/README.md
  • docker/reviewer/bin/reviewer-launch
  • docker/reviewer/runtime-matrix.json
  • docker/reviewer/shims/sfw
  • package.json
  • packages/cli/test/reviewer/ci-job.test.ts
  • packages/cli/test/reviewer/container-diff.test.ts
  • packages/cli/test/reviewer/resolve-runtime.test.ts
  • packages/cli/test/reviewer/reviewer-launch.test.ts
  • scripts/reviewer/a9-red-proofs.sh
  • scripts/reviewer/build-reviewer-image.mjs
  • scripts/reviewer/ci-job.mjs
  • scripts/reviewer/filter-container-diff.mjs
  • scripts/reviewer/image-checks.sh
  • scripts/reviewer/resolve-runtime.mjs
  • scripts/reviewer/reviewer-launch.mjs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

tps-flint and others added 4 commits September 28, 2026 20:19
The build/check steps piped into tee, so the pipeline reported tee's exit
status and the job stayed green while the image build failed. pipefail makes
the lane fail closed on either a failed build or a failed check.
The probe was a temporary red-run demonstration; the image carries no
credential helper. Reverted by a normal commit (no force-push).
…named CI job from parsed YAML

Round 2 of #428 (PR 2 of #425), from the adjudication of 215a845: the build
path resolves the workspace declarations against the fixed runtime table and
the image's baked identity and verifies node/bun before any step; the CI plan
runs every run: step of the host-named job from parsed YAML under bash -eo
pipefail; npm-semver range semantics; named out-of-matrix refusals; child env
from an allowlist; linux/amd64, no entrypoint (OpenClaw 2026.8.1 run model);
permanent A9 red proofs; engines.node "22.x" for this repository.

Refs #425

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@tps-ember
tps-ember force-pushed the feat/425-reviewer-image branch from 031b6bf to e32d9b2 Compare September 28, 2026 21:45
tps-flint and others added 2 commits September 28, 2026 21:49
…d run, close credential and symlink paths

Round 5 of #428 (PR 2 of #425), from the adjudicator's pass 2 at 895c743.

- Launch target: the workflow, job and base branch come only from the
  environment the host gave the sandbox at creation (/proc/1/environ); a caller
  passing different values is refused (assignment-override). No --workspace:
  the worktree is always /workspace. The runtime probe runs
  /usr/local/bin/{node,bun}; steps run /bin/bash with a fixed PATH.
- Planner: requires a pull_request trigger covering the host-named base (exact
  branch names; no path filters; types must include opened and synchronize);
  printable-ASCII keys only; unknown top-level keys refused. Plans the named
  job's needs closure; the launcher runs it dependencies first with CI's gating
  and passes only if every job in the closure succeeded.
- Skipped actions only at reviewed commit SHAs, with value limits where a skip
  would change the outcome.
- Credentials: credential-shaped and config-redirecting workflow env refused;
  each step's effective env re-checked; the worktree's git configuration
  inspected before every job (keys named, values never repeated).
- Lockfiles: symlinked lockfiles and symlinked directories refused before
  execution; a lockfile that becomes a symlink counts as drift; bounded walk.
- Working directories: resolved-path containment immediately before each step.
- Bounds: workflow <= 256 KiB, <= 50,000 YAML nodes (aliases count per use),
  <= 32 levels; declaration files read with size caps inside the worktree.

Refs #425

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tps-flint and others added 2 commits September 29, 2026 05:05
…aseline, post-job alias refusal; review-build-ok is advisory

Round 6 of #428 (PR 2 of #425), from Gauge's pass 3 at e33b48f, under Flint's
scope decision: review-build-ok is evidence for the reviewer, not a merge gate
(CI stays the gate); the boundary held is no credential and no host access
reachable from the review; CI-fidelity gaps are fixed when cheap and otherwise
documented.

- Skipped actions: each needs the inputs whose absence makes the real action
  fail (cache: path and key; upload-artifact: path; setup-bun/setup-node: an
  exact version; socketdev: mode firewall-free) and accepts only values whose
  skip is equivalent (no fail-on-cache-miss, no if-no-files-found: error, a
  valid artifact name unique in the workflow, bounded retention/compression,
  no version ranges, files or caches on setup-*). Every job must begin with
  actions/checkout; a later checkout is refused; checkout is skipped only when
  the worktree is the fresh clone it would produce (no modified, untracked or
  ignored path; depth 1 shallow without tags, or full for fetch-depth: 0).
- timeout-minutes is enforced: each step runs in its own process group, killed
  at its limit or the job's (default 360, capped at 360); what a job leaves
  running is killed when it ends. runs-on accepts only ubuntu-latest and
  ubuntu-24.04.
- Git: the effective configuration must stay inside a documented safe baseline
  (what a plain clone and an identity write); core.fsmonitor, core.hooksPath,
  core.pager, core.editor, protocol.*, uploadpack.*, url.*.insteadOf and every
  other key outside it are refused by key, and repository hooks are refused;
  re-checked before every job.
- After every job, symlinked directories are refused, not only lockfiles.
- Each job gets a fresh HOME/TMPDIR/cache root.
- Docs: review-build-ok is advisory; CI is the merge gate; known fidelity
  limits named; the host assignment's reliance on same-user process-memory
  write denial is a PR 3 host-integration check; "build mode takes no
  arguments"; "no configured credential helpers".
- Image fixtures: an enforced step timeout, a worktree that is not a fresh
  clone, a git core.fsmonitor; every fixture worktree is now a depth-1 clone.

Refs #425

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… pinned HEAD/remote checks, no timeout on skipped steps; qualify the host-assignment and clean-clone claims

Round 7 of #428 (PR 2 of #425), from Gauge's final check at 38ac74d, under the
scope decision in force: review-build-ok is advisory, CI is the merge gate, the
trust boundary is held, and CI-fidelity gaps are fixed when cheap, otherwise
the claim is narrowed and the gap filed.

- Assignment integrity (no code change): the README, changelog and PR body now
  say the host assignment read from /proc/1/environ is immutable to the review
  only if the reviewer host denies same-user process-memory writes, which is
  not yet verified (#436).
- Skipped actions: every input refuses a ${{ }} expression, since a skipped
  action never evaluates it (the counterexample key `${{ runner.os }},bad` is
  refused); restore-keys is held to the same rule as the primary cache key.
  The blanket "refuses expressions" claims now say exactly where.
- Clean-clone check (narrowed from "fresh clone"): before each job it also
  refuses a tracked file carrying the assume-unchanged or skip-worktree bit
  (git ls-files -v), compares against the real objects (--no-replace-objects),
  and requires HEAD and every remote.<name>.url / .fetch value to be what they
  were before the first job. It is not a byte comparison with a host-pinned
  commit; per-job isolation stays in #435.
- timeout-minutes on a skipped uses: step is refused; "each step's timeout is
  enforced" now reads "each planned run: step's".
- The git baseline allowlists keys; remote URL and refspec values are the host
  clone's, pinned for the build rather than approved; the docs say so.

Refs #425

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@tps-flint

Copy link
Copy Markdown
Contributor

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@tps-kern tps-kern left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — checksum-pinned reviewer sandbox image and launcher (PR 2 of #425)

Repo visibility checked: PUBLIC — via gh api repos/tpsdev-ai/cli --jq .visibility, before writing. Nothing below is an unpatched-exposure writeup; my focus areas hold as documented.

Head verified: 9df196ff. Author: tps-anvil (internal) — reviewed from a worktree with full install + the PR's test files.

The planner/launcher split (my focus)

  • Planner — scripts/reviewer/ci-job.mjs: a pure validation/planning library, no execution. It parses a bounded workflow (256 KB / 50k nodes / depth-32 caps, js-yaml CORE_SCHEMA), validates against key allowlists, and refuses first (ci-unhonourable) on every unknown action, ref, or input. Its output is a plan: run: steps (script, merged env, working-directory, timeoutMinutes), skipped steps (ref + reviewed tag, reason, pins, shim, fetchDepth), and the job's timeoutMinutes (default 360 — GitHub's cap).
  • Launcher — scripts/reviewer/reviewer-launch.mjs: owns all execution. It imports the planner as a library (line 77: planJob + the regexes, nothing else) and runs inside the container (bin/reviewer-launch → node /opt/reviewer/lib/reviewer-launch.mjs). Repository-controlled YAML reaches the launcher only through planJob's validated output.
  • The launcher holds its own invariants independent of the plan — hermetic env per job (109–159), the git safe baseline (506–563; --no-replace-objects at 533), checkFreshClone (626+: HEAD^{commit} verified, pin drift against the checkout action's pinned inputs, ls-files -v for assume-unchanged/skip-worktree bits), process-group step-timeout kill (750+), symlink refusals (388–453). The boundary does not depend on the plan being the only defense.

Documented limits match the code — each WHAT claim checked

  • Allowed runners: RUNNER_LABELS (ci-job.mjs:80) — ubuntu-latest/ubuntu-24.04 only; other labels refused.
  • Skipped actions, literal inputs, no expressions: usesStep (382–447) — SKIPPED_ACTIONS only, each only at a reviewed full-SHA ref; per-input validator functions; scalars only; hasExpression(input) → refuse; required inputs must be present.
  • Checkout only first; HEAD pinned: checkout is planned only as a job's first step; its pin inputs flow through the plan into checkFreshClone's pinDrift.
  • Clean clone; replace objects ignored: checkFreshClone (626+) + gitArgs with --no-replace-objects (533); index bits checked via ls-files -v (645).
  • Git config baseline; pinned remote URL/refspec: GIT_BASELINE_KEYS/PATTERNS (506–527) with unsafe values (credentials, programs, transport changes) refused before any step runs (563), and the launcher's findings message redacts embedded userinfo from URLs (527).
  • timeout-minutes: enforced on run: steps and the job deadline (launcher 903–904, 750–773: whole process group killed); refused on skipped uses: steps (ci-job.mjs:629–630) — exactly the documented contract (header line 37).
  • Symlinked directories refused after each job: scan (388–453) + after-job refusal, matching the header contract.
  • Fresh HOME/TMPDIR/cache per job: hermeticLayout/hermeticEnv (109–159), applied per job.
  • #435 (one worktree across a needs closure; per-job cleanup + clean-clone check; node_modules preserved): documented under Known limits with the carry-over risk named, and the launcher's own comment (614) carries the same qualification — stated as strong as the code.
  • #436 (assignment integrity): README states it reads from the container init's environment and that integrity "relies on the deployed sandbox denying same-user process-memory writes… holds only against a caller that sets its own environment, not against one that can write the init process's memory" — matching readCreationEnv (/proc/1/environ) + hostAssignment; the qualification is in the README and the head commit, not hidden.

Verification performed

  • Worktree at 9df196ff; bun install --frozen-lockfile; the four reviewer test files (ci-job, container-diff, resolve-runtime, reviewer-launch): 191 pass / 0 fail.
  • Mutation: neutering the skipped-input ${{ }} check in usesStep fails 1 test in ci-job.test.ts (of 51); reverted, 0 fail — the expression refusal is pinned, not just written down.
  • a9-red-proofs.sh read: the image's no-credential claims have a built-in red-proof harness — planted-credential images must FAIL image-checks or the proof itself fails.

Boundaries (what I did not review)

  • The .github/workflows/test.yml and package.json hunks (CI-lane wiring and script entries) — not read; CI status is the owner's to gate.
  • image-checks.sh (456 lines), the Dockerfile, build-reviewer-image.mjs, and resolve-runtime.mjs internals — credential reach and image integrity are the parallel review's lane; I verified their interfaces and their tests only.
  • I did not run the container or the launch path end-to-end; the evidence is source-reading plus the PR's own tests.

Findings

None in my focus. The two scope-decision gaps (#435, #436) are stated in the README exactly as strong as the code, per the owner's scope decision; the remaining Known-limits entries (merge vs assigned head, runner Node versions, no Socket Firewall, cold cache, pipefail vs bash -e, static assignment/base refusal, repo-supplied credentials out of scope, /tmp noexec) are each visible in the code paths I read or are explicitly environmental.

Verdict: APPROVE

@tps-sherlock tps-sherlock left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE (advisory lane). The trust boundary holds and the documented limits match the code; two non-blocking findings and one nit below.

Reviewed head 9df196ff. tpsdev-ai/cli is PUBLIC (repos/tpsdev-ai/cli .visibility = public); author tps-anvil is a tps-* agent. I built nothing for the image (no Docker here) but installed and ran the JS reviewer suite in a worktree.

What I verified (my focus: credential/host reach, repo-controlled execution, claims == code)

  • No credential in the lane's environments. hermeticEnv (reviewer-launch.mjs:141-150) builds the child env from an allowlist — only LANG/LC_ALL/TERM/TZ from the parent, the hermetic HOME/TMPDIR/caches, a fixed PATH, CI=true; checkStepEnv re-checks every step's effective env (:158-168). forbiddenEnvReason (ci-job.mjs:166-172) refuses credential-shaped and config-redirecting names. The A3 fixture observed leaked= empty and A9 plants+requires red proofs for five credential shapes (a9-red-proofs.sh), which is real backing, not a bare claim.
  • No repo-controlled git config/hook/path escapes. The baseline allowlists keys and refuses everything else wherever it comes from (gitConfigFindings, :511-533), refuses non-*.sample hooks (:566-579), refuses symlinked lockfiles/dirs before and after every job (scanWorkspace, :392-460), resolves each step's working dir inside the worktree (containedDirectory, :709-724), runs fixed /usr/local/bin/{node,bun} and /usr/bin/git with launcher-controlled -c options, never uses the caller's PATH, evaluates no ${{ }} expression, and parses YAML with CORE_SCHEMA (no tags/code).
  • Assignment and clean-clone claims are honest. hostAssignment (:291-317) reads only the container init's /proc/1/environ and refuses a differing caller value; the comment and README both state the integrity is conditional on the host denying same-user ptrace (#436, not in place). checkFreshClone (:625-680) does exactly the four things it says and explicitly disclaims byte-for-byte equality with a host-pinned commit (#435).

Evidence. bun install --frozen-lockfile (EXIT 0) then bun test packages/cli/test/reviewer/ → 191 pass / 0 fail (reviewer-launch 79, ci-job 59, resolve-runtime 27, container-diff 6). Mutations: neutering the assignment-override refusal and zeroing the git-config findings turns reviewer-launch.test.ts red (4 fail), so those properties are test-backed. I did not run image-checks.sh / a9-red-proofs.sh (they need Docker + linux/amd64) or the CI lane — those checks are unverified by me here.

Findings (non-blocking)

[1] scripts/reviewer/reviewer-launch.mjs:959-968 — the verdict does not carry the HEAD it built.

return {
  ok: true,
  status: "review-build-ok",
  image: identity.imageId,
  node: verified.receipt.node,
  ...
  base: assignment.base,
  jobs,
};

checkFreshClone already reads the commit (rev-parse --verify HEAD^{commit}) but it is never reported. Since review-build-ok is advisory evidence a reviewer quotes on a PR, the one JSON line should name the commit it built, so the evidence is tied to a head. Cheap: add head to the verdict.

[2] scripts/reviewer/reviewer-launch.mjs:6 (and the added-reviewer-sandbox-image.md headline) — "no credential and no host access reachable from the review" is broader than the enforced boundary. The enforced scope is stated in the README's last known-limit bullet: the image, the environments the launcher builds, and the worktree's git config. But repository code runs as the sandbox user, and /proc/1/environ — the very file the assignment is read from (:287-307) — is readable by that same user, so a CI step can read whatever the host put in sandbox.docker.env / --env-file, not just the three assignment vars. In the documented deployment that env file carries only the assignment, so there is no credential today; but the unconditional headline should either say the host env-file carries only the assignment, or list this in Known limits alongside /proc/1/environ. Host-integration, PR 3.

[3] nit — scripts/reviewer/reviewer-launch.mjs:503,523-525. USERINFO_RE = /:\/\/[^/@\s]+@/ flags a normal ssh://git@host/… baseline remote as credential-bearing, so a worktree whose clone used an ssh remote URL is refused (an scp-style git@host:org/repo is not). Conservative and thus safe, but a false positive worth narrowing.

Nothing here is an unpatched bypass and the runtime is correct; [1]-[2] are worth a fast follow-up given they touch the evidence and the headline claim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants